Hammer-on and pull-off implementation #28118

mike-spa · 2025-05-23T10:07:19Z

Hammer-on and pull-off implementation

bkunda

It's very lovely! So much about this is delightful to use.

My few bits of feedback at this stage are:

1. Default text size

It'd probably be nice to use 8pt instead of 10pt for the "H"/"P" default size. 10 is just a bit too large IMO.

2. Styles dialog page freeze issues

See video. We've definitely encountered this before, and I suspect it's a product of mixing QML components in the old dialog. Lmk if there's anything we can actually do about this.

3. Select next element in score

See video. Currently this command moves selection to the end of the score (it obviously shouldn't do this).

4. Upper/Lower-case buttons in Styles

We do actually have musescoreicon icons for the upper-case/lower-case buttons. It would be nicer to use these. Also these buttons should have tooltips please (see spec).

miiizen

I've only spotted a few small things so far!

miiizen · 2025-05-28T13:44:31Z

src/engraving/rw/write/twrite.cpp

+
+    xml.startElement(item);
+    if (ctx.clipboardmode()) {
+        xml.tag("stemArr", Slur::calcStemArrangement(item->startElement(), item->endElement()));


This can be removed now we've merged #28152

miiizen · 2025-05-28T13:46:21Z

src/engraving/types/typesconv.cpp

    { ElementType::SYSTEM,               "System",               muse::TranslatableString("engraving", "System") },
    { ElementType::CHORD,                "Chord",                muse::TranslatableString("engraving", "Chord") },
    { ElementType::SLUR,                 "Slur",                 muse::TranslatableString("engraving", "Slur") },
+    { ElementType::HAMMER_ON_PULL_OFF,   "HammerOnPullOff",      muse::TranslatableString("engraving",  "Hammer-on pull-off") },


This is duplicated below

miiizen · 2025-05-28T13:53:28Z

src/notation/view/widgets/editstyle.cpp

    // be triggered by the decimal digit immediately following a non-ASCII character (curly quote).
-    oneMeasureRepeatShow1->setText(muse::qtrc("EditStyleBase", "Show ‘1’ on 1-measure repeats"));
-    singleMMRestShowNumber->setText(muse::qtrc("EditStyleBase", "Show number ‘1’"));
+    oneMeasureRepeatShow1->setText(muse::qtrc("EditStyleBase", "Show â€˜1â€™ on 1-measure repeats"));


Some garble from uncrustify

miiizen · 2025-05-28T13:56:29Z

src/notation/view/widgets/editstyle.cpp

    case ElementType::PARTIAL_TIE:
    case ElementType::PARTIAL_TIE_SEGMENT:
+    case ElementType::HAMMER_ON_PULL_OFF:
+    case ElementType::HAMMER_ON_PULL_OFF_SEGMENT:


See the Github warning below - you're missing a case for TextStyleType::HAMMER_ON_PULL_OFF (1722)

Ah right! (it's also pointing to the wrong page, given that Hopo do have their own page style)

The compiler warnings mentioned here have not been fixed either, despite the comment, so that makes me think there's just a push missing

Sorry, I just forgot about that. See #28226

miiizen · 2025-05-28T13:57:26Z

src/palette/internal/palettecreator.cpp

        static const std::vector<PlayTechAnnotationInfo> playTechAnnotationsMaster = {
-            { QT_TRANSLATE_NOOP("palette", "détaché"),   PlayingTechniqueType::Detache },
-            { QT_TRANSLATE_NOOP("palette", "martelé"),   PlayingTechniqueType::Martele },
+            { QT_TRANSLATE_NOOP("palette", "dÃ©tachÃ©"),   PlayingTechniqueType::Detache },


More uncrustify rubbish

I see this had not been fixed before the PR was merged. I'll fix it now, but it might be good to double-check that that doesn't mean that any other fixes had not been pushed either.

I probably did fix it and then Uncrustify messed it up again as it runs on every save. Do we know if that's an Uncrustify or a Qt Creator bug? I may need to just disable Uncrustify altogether, it's way to easy to accidentally push garbage

Do we know if that's an Uncrustify or a Qt Creator bug?

I don't know. Perhaps one way to find out, is to run Uncrustify from the command line. If it still happens, you could look if there are some Uncrustify options to force Uncrustify to read the file as UTF-8, because maybe that's where it goes wrong.

miiizen · 2025-05-28T14:07:49Z

src/engraving/rendering/score/tlayout.cpp

+        } else {
+            // The last endChord of this segment is in next system. Use end barline instead.
+            Measure* lastMeas = system->lastMeasure();
+            for (Segment* seg = lastMeas->last(); seg; seg = seg->prev()) {


Maybe something like this is more concise

Suggested change

for (Segment* seg = lastMeas->last(); seg; seg = seg->prev()) {

Segment* seg = lastMeas->last(SegmentType::BarLineType);

endX = seg ? seg->systemPos().x() : endX;

Ha, yes thanks, I forgot we had that!

miiizen

Excellent stuff!

Happened in musescore#28118

mike-spa requested review from a user and bkunda May 23, 2025 10:07

mike-spa mentioned this pull request May 23, 2025

Fix grace note after stem slash save #28081

Merged

mike-spa force-pushed the hopo branch from 3864d31 to d846bb0 Compare May 23, 2025 11:20

bkunda reviewed May 26, 2025

View reviewed changes

mike-spa force-pushed the hopo branch 3 times, most recently from 6d1aab2 to 1ce757c Compare May 28, 2025 08:15

ghost approved these changes May 28, 2025

View reviewed changes

ghost requested a review from miiizen May 28, 2025 09:59

miiizen requested changes May 28, 2025

View reviewed changes

miiizen reviewed May 28, 2025

View reviewed changes

mike-spa added 12 commits May 29, 2025 07:39

HammerOnPullOff implementation

0099067

corrections

eef9b7a

Set default HOPO text style to 8pt

9f4a37c

Property panel show Slur setting on select Hopo

6cde4e3

Implement keyboard navigation

70d2bdf

Replace slur with hopo if inserted on top

eaa4eb3

Allow placement of HopoText independent of Hopo

8eef92b

Don't let HP text kern vertically in between notes

324abc0

Handle cross-system hopos correctly

6d6283a

Update MusescoreIcon file and use HP icons

498e558

Improve HP centering on stem side

c769909

Fix utests

897f47e

ghost added this to MuseScore Studio 4.6 May 29, 2025

ghost moved this to In Progress in MuseScore Studio 4.6 May 29, 2025

mike-spa force-pushed the hopo branch from 3201a43 to 0c2b840 Compare May 29, 2025 07:58

Code review

7b61a1a

mike-spa force-pushed the hopo branch from 0c2b840 to 7b61a1a Compare May 29, 2025 08:01

miiizen approved these changes May 30, 2025

View reviewed changes

miiizen merged commit 6b8eac6 into musescore:master May 30, 2025
12 checks passed

github-project-automation bot moved this from In Progress to Done in MuseScore Studio 4.6 May 30, 2025

cbjeukendrup added a commit to cbjeukendrup/MuseScore that referenced this pull request Jun 1, 2025

Fix é characters that were broken by Uncrustify

3a5660d

Happened in musescore#28118

cbjeukendrup mentioned this pull request Jun 1, 2025

Fix é characters that were broken by Uncrustify #28208

Merged

mikekirin pushed a commit to mikekirin/MuseScore that referenced this pull request Jun 10, 2025

Fix é characters that were broken by Uncrustify

a739455

Happened in musescore#28118

	for (Segment* seg = lastMeas->last(); seg; seg = seg->prev()) {
	Segment* seg = lastMeas->last(SegmentType::BarLineType);
	endX = seg ? seg->systemPos().x() : endX;

Hammer-on and pull-off implementation #28118

Hammer-on and pull-off implementation #28118

Conversation

mike-spa commented May 23, 2025

Uh oh!

bkunda left a comment

Choose a reason for hiding this comment

1. Default text size

2. Styles dialog page freeze issues

3. Select next element in score

4. Upper/Lower-case buttons in Styles

Uh oh!

miiizen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miiizen May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miiizen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miiizen May 28, 2025 •

edited

Loading